Repository navigation
fix(diagnostics): open VideoOut telemetry and sampler files under any Unicode path on Windows - #12996
Conversation
… Unicode path on Windows APS5_VIDEOOUT_TELEMETRY_DIR, APS5_SAMPLE_THREADS, APS5_SAMPLE_WORKER and APS5_SAMPLE_COMMITTER were read with getenv and opened with the narrow fopen/ofstream, so on Windows they worked only for characters in the ANSI code page, and the tests, which pass UTF-8 path.string() values, failed whenever %TEMP% was not ASCII. They are now read with _wgetenv and carried as UTF-8, as the other diagnostic directories (ANYPS5_SCREENSHOTS, APS5_DUMP_SUBMITS, APS5_PAD_INPUT_FILE) already are, and the files are opened through std::filesystem::path. The worker sampler test sets its variables with _wputenv_s and reads the results through std::filesystem::path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for fixing the wide environment reads and file opens. One publication step still uses the narrow API: WorkerSampler.cpp:225 calls MoveFileExA(temporary.c_str(), collapsed.c_str(), ...) after the temporary file was created with _wfopen from a UTF-8 path. On code page 1252 the A call interprets those UTF-8 bytes differently, so a Unicode parent directory leaves the .collapsed.txt.tmp file unpublished. This also affects non-ASCII names that previously fit the ANSI code page, since the stored path is now UTF-8. Please convert both paths back to native wide filesystem paths and call MoveFileExW, keeping replacement behavior. Extend the Windows sampler regression to read the final .collapsed.txt (not only the primary worker/process text), check its header/content and that publication consumed the temporary file. Run under both accented and out-of-code-page TEMP paths. The current test only checks the primary text files, so its passing result misses this step. |
…FileExW The collapsed-stacks file is written to a .tmp next to it and then moved over the final name. That move still used MoveFileExA with the UTF-8 path, so under a non-ASCII directory the .tmp was never published. Both paths now go through std::filesystem::path and MoveFileExW, with the same MOVEFILE_REPLACE_EXISTING. The worker sampler test now also reads the published .collapsed.txt, checks its "# samples=N" header with N > 0, and checks that the .tmp is gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
|
@oneandonlydean thanks, you're right. Fixed in
Windows 11 22621 (code page 1252), GCC 15.2 UCRT,
So the extended test catches the unpublished AI-assisted: yes (Claude Code). |
|
Linux run at this PR's exact head Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release,
The non-Windows branch of the new |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The Unicode-path fix and current-main merge check look sound, but this branch conflicts with open #7622 and its checklist still says Depends on: none.
What I ran
- PR head:
ec7d2f5b85ffa83dc05d71051ba824177c1d7531. - Base:
upstream/mainatd820fef1827f27247cd28da7b4eec7891904bae5; current-main merge preview was clean (b79f05d677514ef6af50495a011eaa8e6eb8334a). No touched paths changed onmainafter the reported comparison base58b2753d74. - Environment: Linux x86_64; no local build or platform tests run.
git diff --check upstream/main...refs/review/pr-12996-head
git merge-tree --write-tree upstream/main refs/review/pr-12996-head
python tools/check_conventions.py --base upstream/main --head refs/review/pr-12996-head
git merge-tree --write-tree refs/review/pr-12996-head refs/review/pr-7622-head
git diff --name-only 58b2753d74...upstream/main -- core/libs/prx/libSceAgcDriver/Execution/src/WorkerSampler.cpp core/libs/prx/libSceAgcDriver/tests/WorkerSampler.cpp core/libs/prx/libSceVideoOut/src/Telemetry.cppResults
| Check | Result |
|---|---|
| Patch whitespace | Pass; command exited 0 with no output. |
| Current-main merge preview | Pass; clean tree b79f05d677514ef6af50495a011eaa8e6eb8334a. |
| Repository conventions | Pass. |
| Pairwise integration with #7622 | Conflict in core/libs/prx/libSceAgcDriver/Execution/src/WorkerSampler.cpp; both PRs remain open. |
| Hosted CI | Windows product, eligibility, conventions, labels, and report checks passed; Linux and specialized jobs were skipped. |
| Platform tests | The PR reports Windows non-ASCII TEMP tests passing 4/4 in three runs, including the collapsed-file publication check. It also reports the Linux telemetry test passing. These results are author-reported, not independently reproduced here. |
Unedited local command output
python tools/check_conventions.py --base upstream/main --head refs/review/pr-12996-head
All conventions checks passed.
git merge-tree --write-tree upstream/main refs/review/pr-12996-head
b79f05d677514ef6af50495a011eaa8e6eb8334a
git diff --check upstream/main...refs/review/pr-12996-head exited 0 with no output. The comparison of changed paths since 58b2753d74 also produced no output.
The pairwise merge preview exited 1 and reported:
CONFLICT (content): Merge conflict in core/libs/prx/libSceAgcDriver/Execution/src/WorkerSampler.cpp
Findings
Blocking
- Undeclared overlap with #7622: The pairwise merge preview conflicts in
WorkerSampler.cpp, while this PR saysDepends on: none. The current repository rule requires authors to agree which overlapping PR goes first and record that order withDepends on(pinned contribution rule). Please coordinate the order with #7622's author and update the dependency line; then I can recheck the combined merge.
Non-blocking
- No additional blocker found in the source-level review. The reported Windows and Linux test runs were not independently reproduced here.
Not verified
- I did not build or run platform tests locally. Linux and specialized hosted jobs were skipped.
To get this to a good state
Please record the agreed order for the overlapping PRs in Depends on. I will rerun the merge checks and review the updated head.
|
@MarcZX10 thanks. The overlap with #7622 is a single line. In I propose this PR lands first: it's three files, and #7622 is much broader. #7622 then resolves that line to AI-assisted: yes (Claude Code). |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 427 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The Windows path publication fix and regression are present, the proposed overlap order is now agreed, and the current-main preview and non-build checks pass.
What I ran
- Exact PR head:
ec7d2f5b85ffa83dc05d71051ba824177c1d7531; PR base:184afa49adce16e298c47d136706fd2356198852. - Compared against current
main5ecef095b61e7a406b279c97896f3a8a6fbd6353; the preview merged cleanly as tree76a793b90747a855c7490f47727855dd0b8a7793. - Environment: Linux x86_64; no compiler or local build/test run.
git merge-tree --write-tree upstream/main refs/review/pr-12996-current
git diff --check upstream/main...refs/review/pr-12996-current
python tools/check_conventions.py --base upstream/main --head refs/review/pr-12996-current
git diff --name-status 58b2753d74...upstream/main -- core/libs/prx/libSceAgcDriver/Execution/src/WorkerSampler.cpp core/libs/prx/libSceAgcDriver/tests/WorkerSampler.cpp core/libs/prx/libSceVideoOut/src/Telemetry.cppResults
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 76a793b90747a855c7490f47727855dd0b8a7793 |
git diff --check |
Passed; no output |
| Repository conventions | Passed |
| Changed paths since reported comparison base | None in the three relevant paths |
| Hosted Windows job | Passed on exact head |
| Hosted Linux product job | Skipped |
| Focused Windows and Linux tests | Author reports the Windows Unicode-TEMP cases and Linux telemetry test passed on exact head; I did not independently run them. |
| Overlap coordination | I agreed in the PR thread to #12996 first, then #7622; this is merge order only. |
Unedited local command output
$ git merge-tree --write-tree upstream/main refs/review/pr-12996-current
76a793b90747a855c7490f47727855dd0b8a7793
$ python tools/check_conventions.py --base upstream/main --head refs/review/pr-12996-current
All conventions checks passed.
git diff --check and the changed-path comparison exited 0 with no output.
Findings
Blocking
None found. The publication path now uses MoveFileExW with native wide paths, and the Windows regression checks the collapsed file and temporary-file removal under Unicode temporary paths.
Non-blocking
Linux product CI is skipped. The focused Linux test result is author-reported, not independently run here.
Not verified
I did not run local builds or tests, and did not test on macOS or console hardware.
|
Windows run with a non-ASCII Windows 11 Pro 10.0.22631 (code page 1252), i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Release,
This matches your Separate from this PR: 16 other tests abort under the AI-assisted: yes (Claude Code) |
|
Checked the wide env reads, the |
What
Four diagnostic paths were read with
getenvand opened with the narrowfopen/ofstream:APS5_VIDEOOUT_TELEMETRY_DIRin VideoOut, andAPS5_SAMPLE_THREADS,APS5_SAMPLE_WORKERandAPS5_SAMPLE_COMMITTERin the AGC worker sampler. On Windows both sides use the ANSI code page, so these worked only for characters in it. A directory such asC:\x\東京テストon a code-page-1252 system could not be opened. Their tests also fail whenever%TEMP%is not ASCII, because they pass a UTF-8std::filesystem::path::string()into those APIs (follow-up to #12993, which fixes the pure test bugs).The other diagnostic directories already read the wide environment and use
std::filesystem::path:ANYPS5_SCREENSHOTS,APS5_DUMP_SUBMITS,APS5_DUMP_COMPUTE_PIPELINES,APS5_PAD_INPUT_FILEandAPS5_TRACE_CALLS_FILE. This makes these four match:Instance()takes the directory from_wgetenv(L"APS5_VIDEOOUT_TELEMETRY_DIR")as UTF-8 (getenvelsewhere).Writeropensvideoout.jsonlthroughstd::filesystem::path(u8 directory). Itsstd::stringdirectory is UTF-8 on every host, so the existing test, which passesdirectory.string(), now works unchanged._wgetenv(L"…")and kept as a UTF-8std::string, and the three output files, including the collapsed-stacks temporary, are opened with_wfopenthroughstd::filesystem::path, and the temporary is published withMoveFileExW(wasMoveFileExA, found in review). Its test now sets the variables with_wputenv_sand reads the files throughstd::filesystem::path, instead of narrow strings that the C runtime would read as ANSI. In the worker modes it also checks the published.collapsed.txtheader and that its.tmpis gone.Nothing changes for an ASCII path.
Tested
Windows 11 Enterprise 10.0.22621 (code page 1252), WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
BUILD_TESTING=ON, no Vulkan driver visible, this head (ec7d2f5b85) onmain58b2753d74.TEMPmain184afa49adC:\x\tëmpünïC:\x\東京テスト(outside code page 1252)TEMPThe four tests are
video_out_telemetry,agc_worker_sampler_default,agc_worker_sampler_selectedandagc_worker_sampler_process(ctest --repeat until-fail:3). The full build succeeded at the first head (74ec7311f1); the review fix touches onlyWorkerSampler.cppand its test, whose targets build at this head. With the previousMoveFileExAand the new test,agc_worker_sampler_defaultand_selectedfail under both non-ASCIITEMPvalues, so the test covers the publication step.python tools/check_conventions.py --base origin/main: all checks passed. Each variable is still read by a literal_wgetenv(L"…")call, so thedocs/dev/ENVIRONMENT.mdreference check keeps finding them._WIN32block. Telemetry there now opens throughstd::filesystem::pathof the same bytes and still readsgetenv.Independent Windows validation, reported by bunbeans2-arch for topic
ec7d2f5b85ffa83dc05d71051ba824177c1d7531against mainf7f03daf52: Windows 11 Pro 22631, i9-9900K, MSYS2 UCRT64 GCC 16.1, Release. Four telemetry/sampler tests pass 3/3 for each of theéand outside-code-pageЖtemporary paths, while main fails 3/3; normal-profile controls pass. The separately reported hoststd::tmpfile()failures concern #13083/#13104. This is the contributor's run, not a new local Windows execution.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)Libc::StartHostJThreadwrapper and this PR’s[path = *path]capture; author confirmation)🤖 Generated with Claude Code